[PGO] Add llvm.loop.estimated_trip_count metadata #152775
Conversation
This patch implements the `llvm.loop.estimated_trip_count` metadata discussed in [[RFC] Fix Loop Transformations to Preserve Block Frequencies](https://discourse.llvm.org/t/rfc-fix-loop-transformations-to-preserve-block-frequencies/85785). As [suggested in the RFC comments](https://discourse.llvm.org/t/rfc-fix-loop-transformations-to-preserve-block-frequencies/85785/4), it adds the new metadata to all loops at the time of profile ingestion and estimates each trip count from the loop's `branch_weights` metadata. As [suggested in the PR#128785 review](#128785 (comment)), it does so via a `PGOEstimateTripCountsPass` pass, which creates the new metadata for the loop but omits the value if it cannot estimate a trip count due to the loop's form. An important observation not previously discussed is that `PGOEstimateTripCountsPass` *often* cannot estimate a loop's trip count but later passes can transform the loop in a way that makes it possible. Currently, such passes do not necessarily update the metadata, but eventually that should be fixed. Until then, if the new metadata has no value, `llvm::getLoopEstimatedTripCount` disregards it and tries again to estimate the trip count from the loop's `branch_weights` metadata.
Somehow, on some of my builds, `llvm::` prefixes are dropped from some symbol names in the printed past list.
For estimated trip count metdata.
Suggested at <#148758 (comment)>.
jdenny-ornl
requested review from
MatzeB,
arsenm,
nikic,
fhahn,
mtrofin and
jdoerfert
|
This resurrects PR #148758. I'll address previous issues shortly. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I believe I have addressed most issues raised in PR #148758. For now, this PR shows what the pass pipeline tests (e.g., I have not addressed the general issue raised there about whether pgo-estimated-trip-counts is actually worthwhile and whether we should constrain it or disable it by default. What do people think now after seeing the above changes? |
|
|
I think we should at least start by not having the pass and only doing on the fly updates. I assume that this patch is intended to be NFC-ish, but it does cause codegen changes, and as-is it's hard to tell whether that's because of the new loop metadata that is present everywhere, or due to divergence between branch weights and the metadata. (A sample to look at would be libclamav_nsis_LZMADecode.c from llvm-test-suite.) |
jdenny-ornl
force-pushed
the
users/jdenny-ornl/pgo-estimated-trip-count
branch
from
ff9730e to
e7eb1fe
Compare
Thanks. I took a look at libclamav_nsis_LZMADecode.c, using the results you previously sent as a reference. For -O3, that shows that the previously landed version of this PR reduced the instruction count by 25.84%. Following the instructions at the About page there, I used For the current PR, I saw the following summary: I commented out all runs of PGOEstimateTripCountsPass and saw: So running PGOEstimateTripCountsPass still has roughly the same impact as in the previous results: 24.4% reduction. I then tried to determine what in PGOEstimateTripCountsPass causes the change. In setLoopEstimatedTripCount's addStringMetadataToLoop call, I renamed the loop metadata from So, it appears that it is the up-front loop metadata creation that produces the change rather any changes related to estimated trip counts or branch weights. To rule out anything else from the PR, I went back to the main branch and added a new function pass into the pipeline where this PR adds PGOEstimateTripCountsPass. The new pass merely calls addStringMetadataToLoop to add the bogus "foo" loop metadata to all loops. For any function containing loops, it preserves only CFGAnalyses and LazyCallGraphAnalysis, and it preserves all analyses otherwise. I then removed the addStringMetadataToLoop call from that pass to confirm that it not the analysis invalidation causes the change:
Do you think we should remove the pass or just disable it by default, as was previously suggested? I am leery of the latter because I am afraid it will just end up being unused code, unless someone else commits to continuing to investigate it. |
This patch implements the
llvm.loop.estimated_trip_countmetadata discussed in [RFC] Fix Loop Transformations to Preserve Block Frequencies. As suggested in the RFC comments, it adds the new metadata to all loops at the time of profile ingestion and estimates each trip count from the loop'sbranch_weightsmetadata. As suggested in the PR #128785 review, it does so via a newPGOEstimateTripCountsPasspass, which creates the new metadata for each loop but omits the value if it cannot estimate a trip count due to the loop's form.An important observation is that
PGOEstimateTripCountsPassoften cannot estimate a loop's trip count, but later passes can sometimes transform the loop in a way that makes it possible. Currently, such passes do not necessarily update the metadata, but eventually that should be fixed. Until then, if the new metadata has no value,llvm::getLoopEstimatedTripCountdisregards it and tries again to estimate the trip count from the loop's currentbranch_weightsmetadata.